Skip to content

ADFA-4128 (11/11): app + bench — wiring Quick Build into the IDE and the benchmark harness - #1723

Open
fryanpan wants to merge 9 commits into
feature/ADFA-4128-qb-10-gradle-pluginfrom
feature/ADFA-4128-qb-11-app
Open

ADFA-4128 (11/11): app + bench — wiring Quick Build into the IDE and the benchmark harness#1723
fryanpan wants to merge 9 commits into
feature/ADFA-4128-qb-10-gradle-pluginfrom
feature/ADFA-4128-qb-11-app

Conversation

@fryanpan

@fryanpan fryanpan commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Part 11/11 of the stacked split of #1669 (requested by Akash). Base: feature/ADFA-4128-qb-10-gradle-plugin. Stack overview + review mechanics: PR 1 (#1713). Terms are defined in quickbuild/README.md (lands in PR 1).

Puts Quick Build in front of the user: a button next to Run, and enough narration to tell what it is doing and when it has finished. It also adds a harness to make it easier to run standard Gradle build and Quick Build benchmarks, and to gather key metrics about the stages of the build process.

flowchart TB
    subgraph appc["<b>This PR: inside app/ — wiring and bench</b>"]
        act["QuickBuildAction<br/>registered only when<br/>FeatureFlags.isExperimentsEnabled<br/><i>QuickBuildAction.kt</i>"] --> mgr["QuickBuildManager<br/>session lifecycle, provisioning,<br/>stop-tap cancellation"]
        mgr --> narr["QuickBuildOutputNarrator<br/>attached to the session manager;<br/>queues while no pane is bound<br/><i>QuickBuildOutputNarrator.kt</i>"]
        mgr --> sb["status bar collector<br/>lifecycle-scoped: state, not history<br/><i>QuickBuildStatusBar.kt</i>"]
        koin["QuickBuildModule (Koin)<br/>binds every core port;<br/>assetsLiveReloadable read once<br/>at the Android edge<br/><i>QuickBuildModule.kt</i>"]
        tr["bench trampoline activity<br/>debug-source-set manifest only<br/><i>QuickBuildBenchActivity.kt</i>"] --> mgr
        mgr --> hooks["QuickBuildBenchHooks<br/>inert release twin<br/><i>debug/QuickBuildBenchHooks.kt</i>"]
        hooks --> rec["event + metrics recorders"]
        rec --> log["bench-events.jsonl<br/><i>BenchEventsFile.kt</i>"]
        hooks --> e2e["MODE_STANDARD_E2E<br/>measures the standard build<br/>through install + launch<br/><i>QuickBuildBenchAutostart.kt</i>"]
    end
    adb["adb shell am start<br/>gated on android.permission.DUMP"] --> tr
    mgr --> core[":quickbuild:core session manager (PRs 5-8)"]
    narr --> pane["Build Output pane (existing)"]
    sb --> bar["bottom status bar (existing)"]
    mgr -- "provisioning + rebuild builds" --> gbs["GradleBuildService (existing)"]
    classDef thisPrBox fill:#dbeafe,stroke:#93c5fd,color:#1e3a5f
    classDef inPr fill:#ffffff,stroke:#64748b,color:#000
    class appc thisPrBox
    class act,mgr,narr,sb,koin,tr,hooks,rec,log,e2e inPr
Loading

What to review

  • QuickBuildAction.kt — owns the session tap: start, stop-tap cancel, grey-out. Line-by-line.

  • Gradle tuning — Metaspace 192→384 MB + daemon idle timeouts (30 min balanced / 2 h high-perf); the only changes to non-QB behavior

  • QuickBuildOutputNarrator.kt, QuickBuildStatusBar.kt — queued narration; lifecycle-scoped status showing state, not history.

  • QuickBuildModule.kt — binds every core port; reads assetsLiveReloadable at the Android edge.

  • GenerateSourcesDeferral.kt — defers resource-XML generateSources until Quick Build goes idle.

  • John's items C15, C16, C17, C22, C23 folded in as fixes.

  • Rollback: without the flag there is no UI entry point.

  • Followup, not fixed: R8 emits kotlin.Metadata warning noise.

  • QuickBuildBenchAutostart.kt — MODE_STANDARD_E2E measures the standard build through install and launch. Line-by-line.

  • The e2e latch bypasses install confirmations only for the measured span.

  • QuickBuildBenchActivity.kt, QuickBuildBenchHooks.kt — DUMP-gated trampoline; inert release twin.

  • BenchEventsFile.kt — a failed relaunch omits relaunchOk rather than recording zero.

How this PR Was Tested

  • Automated tests (see coverage details below)

  • Manual QA — walked the manual-qa.md test plan on the A56 [measured on a56]

  • Benchmark — measured on real devices, both arms: a warm code edit reaches the running app with about a 5x median speedup over a standard build + deploy. The weaker the phone, the bigger the win. MODE_STANDARD_E2E drove the standard arm through install and launch.

  • Still open — the rebaseline relaunch path is not yet device-verified, and neither is the API 28/29 resource-swap success path.

Coverage (JaCoCo at the stack tip, single run):

A lot of this was UI code and wasn't covered very well by automated tests.

Package Line Branch Note
actions/build 0.5% 0.0% UI — device-tested
actions/file 0.0% 0.0% UI — device-tested
activities/editor 0.9% 1.8% UI — device-tested
analytics/quickbuild 49.6% 30.4%
app 0.7% 0.5% Application classes, Android-bound
di 0.0% 0.0% Koin wiring
fragments/sidebar 0.0% 0.0% UI — device-tested
handlers 7.6% 0.0% Android-bound
quickbuild 27.3% 24.3% mixed UI/logic; see caveat
services/builder 6.8% 0.0% bound Service, Android-bound
utils 0.0% 0.0% mixed UI/logic
viewmodel 0.9% 0.0% UI — device-tested
TOTAL (all app files in this PR) 16.9% 15.3% 2,989 lines, 1,582 branches

Review fixes (2026-08-22)

A review-fixes commit addresses the code-review findings. Two changes here deliberately ship to all users, with the Experiments flag off (approved):

  • GradleBuildTuner. Benefit: builds stop dying with out-of-memory errors on ordinary phones, and a rebuild shortly after a build is much faster. Two settings on every Gradle build: (a) the Metaspace cap (JVM memory for loaded class definitions) goes 192 MB -> 384 MB -- real Android-plugin builds exceed 192 and were being killed mid-build; it's a cap, not a reservation, so no extra memory is used unless the build needs it. (b) The Gradle daemon (the background process that keeps builds warm) stays alive for a time matched to the phone's tier -- 15 min low-memory, 30 min balanced, 2 h high-performance -- so a quick rebuild skips the cold start while weak phones don't host a resident daemon for hours.
  • generateSources narrowing. Benefit: fewer surprise build stalls while editing, less battery and CPU burned. The IDE used to launch a Gradle generate-sources step after every save-all and any XML save, even when nothing that step produces could have changed. Now it runs only when the saved file can actually affect generated sources. Same results, far fewer builds.

One candidate followup from review (orchestrator forcing a full-changed compile after a failed dex/deploy) was re-checked and refuted at this tip: the forced flag re-arms and a forced no-op already performs the full rebuild. The daemon-side recovery lever stays in as defense in depth.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Kj9YeCDHGp9DU8LPtfWJ7W

@fryanpan
fryanpan force-pushed the feature/ADFA-4128-qb-11-app branch from 5b48f90 to c69d8ef Compare August 22, 2026 06:41
@fryanpan
fryanpan force-pushed the feature/ADFA-4128-qb-11-app branch from c69d8ef to 5a3d5eb Compare August 22, 2026 07:06
@fryanpan
fryanpan marked this pull request as ready for review August 23, 2026 02:31

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@fryanpan
fryanpan force-pushed the feature/ADFA-4128-qb-11-app branch from 5a3d5eb to ac3ab4e Compare August 24, 2026 14:44
@fryanpan
fryanpan force-pushed the feature/ADFA-4128-qb-11-app branch from ac3ab4e to a45a359 Compare August 24, 2026 14:48
@fryanpan

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review skipped: 106 files exceed the limit of 100.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

fryanpan and others added 4 commits August 26, 2026 23:44
…adds the debug-only benchmark harness

The app wiring and the bench surface land together because they are mutually
dependent: :app's ProjectHandlerActivity and QuickBuildModule call into
QuickBuildBenchHooks, and QuickBuildBenchHooks returns AutostartBuild and resolves
EnvironmentQuickBuildPaths. Neither ordering of a two-PR split compiles.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj9YeCDHGp9DU8LPtfWJ7W
…ent, test gaps

Important 1 (daemon idle-timeout/Metaspace tuner un-gated): kept un-gated by
design — the 384m Metaspace floor fixes real OOM-killed builds and the tiered
idle timeouts keep low-RAM devices from losing the IDE to lmkd; GradleBuildTuner
now states this in its KDoc. Ships flag-off; needs Bryan sign-off in the PR body.

Important 2 (generateSources narrowing un-gated): judged a genuine all-users
improvement, not QB-specific — the old code ran a Gradle generateSources after
EVERY save-all (and after any XML save in SaveFileAction), a per-save build tax;
flag-off the deferral degenerates to the same immediate call, so the narrowing
is the only behavior change. Known trade (manifest-only edits leave generated
Manifest/R intermediates stale until the next resource save or build) now
stated at both call sites. Ships flag-off; needs sign-off in the PR body.

Important 3 (install dropped on rotation, flag on): installApk's async path now
re-arms AwaitingInstall (BuildViewModel.reArmInstall, fires only from Idle)
from the coroutine's drop path, so a configuration change during the APK-manifest
parse makes the recreated activity's collector retry the install instead of
silently losing a successful build. Covered by BuildViewModelInstallReArmTest.

Test gap (zip-slip guard): extraction loop extracted to
QuickBuildArtifactStager.extractDaemonZip(InputStream, File); the guard is
watched going red by QuickBuildArtifactStagerTest (a ../ entry throws and
nothing lands outside the daemon dir).

Test gap (InstallationEventFlow mapping): InstallationEventFlowTest pins the
PackageInstaller status mapping, including the ABORTED-vs-FAILURE branch order
and the no-extras / no-status paths.

Test gap (service-side output capture): suppress/capture/drain routing
extracted from GradleBuildService.logOutput into InternalBuildOutputCapture;
bounded tail, drain-clears, throwing progress listener and editor-listener
routing pinned by InternalBuildOutputCaptureTest.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj9YeCDHGp9DU8LPtfWJ7W
…tiple

The Gradle daemon idle-timeout comment quoted a speedup multiple, which put a
benchmark figure into shipping production code. The reason the timeout is
generous is structural - a warm daemon skips the cold start, which dominates a
short rebuild - so the comment now says that instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj9YeCDHGp9DU8LPtfWJ7W
- F1723-1 put QuickBuildPipelineTest into the suite that actually runs
- F1723-4 guard the deferred prebuild fire() against a throw

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
@fryanpan
fryanpan force-pushed the feature/ADFA-4128-qb-11-app branch from a45a359 to 7c72105 Compare August 27, 2026 17:32
fryanpan and others added 5 commits August 27, 2026 15:04
The script has had one commit and predates several behaviour changes, so a
walker following it literally hits steps that cannot reach their stated end
state. None of these are product defects - the walk found no product failure.

- T7 criteria 4/5 described the pre-b6cddf035 world where rebaseline left the
  app un-relaunched and a tap was needed. Rebaseline relaunches now.
- T7b step 2 and T11 step 3 end at a modal OS install prompt that never times
  out; "do not tap anything" could not reach the end state.
- T1 gains a FAB baseline tap. Five later tests assert through the FAB, so a
  dead FAB failed them all with no way to tell when it broke.
- T1 gains a note that project creation already ran a setup build, so the first
  tap measures warm provisioning, not cold.
- T14's "no reinstall unless the bytes changed" inverted the design: the
  generation stamp lives in the APK, so a restart always mints new bytes.
- T20 names service-app; only 4 of 30 corpus apps declare a Service.
- T21 drops "wrap and push sora-editor-full first" - already wrapped, with all
  288 source files.

Adds a "Traps that make the product look broken" section for the four method
errors that produced wrong findings: tapping the geometric centre of a view
that extends under a system bar, Find-in-file being a regex search, relaunching
CoGo via monkey when it declares two LAUNCHER activities, and selecting a
wrapped corpus copy by mtime when the newest is pinned to AGP 9.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
Six places where a Quick Build doc contradicted the code it describes. Each was
re-verified against the source rather than taken from the review comment.

- debugging.md: the deploy round-trip row said the 15 s bound covers "one AIDL
  onPayload call". IQuickBuildTarget is a oneway interface, so that call returns
  immediately; DeployChannel wraps the call plus the wait for a
  generation-matched report, which is what its own KDoc already said.
- why-not-android-jar.md: listed native libs as hot-loadable. A .so under
  jniLibs forces a Gradle fallback (ChangeClassifier). Loadable at runtime and
  changeable via live reload are different properties.
- reliability-gaps.md: "five user-facing defects" against three fixed and four
  open. Seven were surfaced; the fixed three are relink-stuck, #88 and #90. Also
  states why Blocks v1? reads TBD - the decision at the top is a proposal, and
  the cells become "No" when it is confirmed.
- low-spec-devices.md: stated an inferred mechanism (SerialGC thrashing in a
  small heap) as the confirmed cause of the 1.9 GB failure. The outcome is
  measured; the mechanism is not, and the uncapped run that would confirm it is
  still unmeasured. Retitled to what was actually observed.
- concurrency.md: the tap-races-its-own-save section read as current behaviour.
  It describes the pre-2026-08-13 design that the redesign below it replaced.
- perf-roadmap.md: incomplete sentence.

Not applied: CodeRabbit's finding that manual-qa.md's screenrecord
--time-limit 1740 is invalid because AOSP caps at 180 s. False on our hardware -
recordings of 1774 s, 2432 s, 2592 s, 2842 s and 3534 s have all completed on
the A56, and the surrounding comment already documents the real 30-minute cap
that 1740 sits under. Applying it would break working recordings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
None of these is a Quick Build failure - the walk passed every test. They are
places where the product is correct and unhelpful.

A2 - the bolt read identically to a screen reader in READY and ERROR. Every tone
has its own icon shape, so a sighted user can tell them apart; collapsing them
all to "Quick Build" hid that distinction from exactly the user who cannot see
the icon. ERROR, SLOW and RECONNECTING now announce their state. BUILDING and
the standard-build-blocked case already did.

A3 - after an undeliverable build the bar read "built, but could not be
delivered - see Build Output" on every poll, while the sentence naming the fix
("Your app is not running. Tap Quick Build to start it with your changes.") was
only in Build Output. The bar now names the tap when that is the whole problem.
Carried as a typed flag rather than matched on the message text, the same way
proxyAppNotConnected already is, and kept separate from it because they mean
opposite things: appNotRunning is "nobody opened it", proxyAppNotConnected is
"we launched it and it still did not arrive".

A4 - an orphaned proxy app reported CoGo's expected connect() rejection at W on
every attempt of the rebind backoff loop, 14 times in one restart window. The
behaviour is right (it continues standalone); repeating an expected rejection at
W buries the entries around it. Reported once per streak now, cleared by a
successful connect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
The page proposed that #87, #89, #91 and the relink-crash gap go to v1.1,
then left the table's "Blocks v1?" column reading TBD on all four rows. A
proposal in a title and a TBD in a table say different things to a reader,
and CodeRabbit flagged the pair as an internal inconsistency.

Decision confirmed 2026-08-25: none of the four block v1. The four cells now
read "No - v1.1", the title states the answer rather than asking it, and the
prose no longer describes itself as awaiting confirmation.

No change to any gap's evidence, root cause, or fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FstXxJ5cwWPcvmhZ9vJgJ7
…ing branch

F1713-3 (docs/concurrency.md): the thesis said every expensive thing runs
in another process while the table directly below it put the mtime poll
and the install call on Dispatchers.IO inside CoGo. Name the exception.

F1713-8 (docs/manual-qa.md): files are not killed, processes are - and a
teammate follows this runbook literally while holding a half-recorded QA
session. Say screenrecord.

Both patch text that exists only in the four trailing commits, so they
could not ship until those commits had a home.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xsc7AMGBVyEMfrwpZX87iC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant